Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Hi @Eh2406!

I'm ready for review.

I'm new to Rust, so please check my code modifications extra carefully 😂

You got me a bit nervous 😅

Opinionated how? Is he against it?

Thanks for the PR!

I would personally prefer to not run clippy on CI. My personal opinion is that it generates far too much noise to be useful and encourages the wrong antipattern by sprinkling #[allow] annotations with seemingly random names all throughout the codebase. I don't mind taking PRs to merge improvements every now and then, but I do not want to force all authors for all PRs to work with the pedantry of clippy.

This is just my personal opinion, though, and others on the team may feel different.

Hi @alexcrichton!

My previous experience in open source was contributing to CocoaPods and implementing its CDN registry client/static site. During that work, I've become very used to Rubocop, the Ruby linter. They have it on the CI and it mainly helps with having the code style be consistent.

Allow me to address the concerns you raised:

• The anti-pattern of allowing violations can and should be enforced by reviewers. Generally speaking, a conservative policy of "no #allowss unless absolutely necessary" goes a long way.
• The pedantry of Clippy can be reduced by adding global #allows`s. In fact, I believe lots of them are already there!
• There's currently less pedantry than it seems. After turning Clippy on CI, I've managed to get rid of all the warnings in a matter of a couple of hours. Maintaining this state should be even less arduous.

To summarize - it will help enforce a consistent style and cleaner code. If rustfmt is enforced, Clippy might as well. Plus, it's way easier to appease than the borrow checker 😂 As Rust continues to gain popularity, the number of contributors will increase. We might as well be ready.

Also, having a completely problem-free output in VS Code is exhilarating!

I understand the value of consistency, yeah, and I can see how clippy tries to nudge to more consistent code. That being said I disagree with many of clippy's more pedantic lints, I personally do not like having a massive list of #[allow] at the top of the crate for just clippy, and already in this PR lots of #[allow] is creeping in.

Again, this is just my personal opinion, but I trust development and ongoing maintenance of rustfmt more than I trust the development and maintenance of clippy. The rustfmt style was agreed upon the community through an RFC process and agreement amongst a team, whereas I believe the clippy lints have not. They're simply "if someone implemented it we threw it in and r+'d it".

I should point out though that it's pretty unlikely that I'm going to be convinced to like clippy any time soon. I am not the only maintainer of this project though and while I would push back against adding clippy to CI if everyone else would like it then I'd of course be ok. I'd like more team input about running this on CI first.

So what do you suggest the next steps should be for this PR?

Also, are you folks okay with rebasing and editing commit history and/or deleting commits from a PR? Or should I add commits on top even if they're basically deletions?

☔ The latest upstream changes (presumably #7575) made this pull request unmergeable. Please resolve the merge conflicts.

Yes rebasing and editing the history is fine. I don't think we want to land the CI support, but landing changes which take into account clippy lints is fine. It's preferred to put any #[allow], if required, at the top of the crate rather than on the use site.

I've revised the suggested changes. For now, I've kept the Clippy CI because it's the easiest way for me to know that all my changes are good, Clippy-wise 🙃

What other things need to happen for allowing the linting to be performed by CI? The benefits are clear and I think shouldn't be ignored.

We discussed this at the Cargo team meeting yesterday. We are not going to run Clippy in azure at this time. If you can add a commit removing the changes to azure-pipelines.yml we are happy to merge the other changes. (I'd prefer to keep it in the history as it looks like a valuable list of all the things that needs to be done to check with clippy.) We don't think it is controversial to say that clippy is at times pedantic and needs a large numbers of #![allow(clippy:: to make acceptable. We discussed some changes that may improve the signal/noise, but it is more an issue for the clippy devs. We acknowledge that it is not a good look that official Rust projects are not willing to dog food the official linter. So we will be bringing this up with the broader Dev-Tools team. It may be something that is best hashed out in person, so we may rezerve time at the Rust All Hands for this discussion.

@Eh2406 Done!

Thank you for working on this!

@bors r+

📌 Commit 1432651 has been approved by Eh2406

⌛ Testing commit 1432651 with merge 3738e1d...

☀️ Test successful - checks-azure
Approved by: Eh2406
Pushing 3738e1d to master...